Make uv’s first-index strategy more secure by default by failing early on authentication failure#12805
Conversation
324c1b4 to
f57e40c
Compare
da2c4d8 to
b694b92
Compare
f57e40c to
5e6f511
Compare
b694b92 to
d7b6799
Compare
5e6f511 to
f7980a8
Compare
| type FxOnceMap<K, V> = OnceMap<K, V, BuildHasherDefault<FxHasher>>; | ||
|
|
||
| #[derive(Debug, Clone, PartialEq, Eq, Hash)] | ||
| pub(crate) enum FetchUrl { |
There was a problem hiding this comment.
Rustdoc please for the variants.
| fn fmt(&self, f: &mut Formatter) -> std::fmt::Result { | ||
| match self { | ||
| FetchUrl::Index(index) => Display::fmt(index, f), | ||
| FetchUrl::Realm(realm) => Display::fmt(realm, f), |
There was a problem hiding this comment.
Nit: Self instead of FetchUrl
| } | ||
|
|
||
| impl Index { | ||
| pub fn is_prefix_for(&self, url: &Url) -> bool { |
There was a problem hiding this comment.
Nit: Rustdoc please with an example of when this returns true vs. false.
There was a problem hiding this comment.
Because of the timing of the release, can we defer this to a PR afterwards? I just moved this logic from a function to a method in this PR.
| } | ||
| _ => Err(ErrorKind::WrappedReqwestError(url, err).into()), | ||
| }, | ||
| ErrorKind::WrappedReqwestError(url, err) => { |
There was a problem hiding this comment.
I'm still struggling a bit. Why do we break when (e.g.) we hit a 401, but 401 isn't included in the list of ignored errors? Why is it not something like this?
diff --git a/crates/uv-client/src/registry_client.rs b/crates/uv-client/src/registry_client.rs
index 8bdf2c065..3cb6b4978 100644
--- a/crates/uv-client/src/registry_client.rs
+++ b/crates/uv-client/src/registry_client.rs
@@ -348,12 +348,6 @@ impl RegistryClient {
}
// Package not found, so we will continue on to the next index (if there is one)
SimpleMetadataSearchOutcome::NotFound => {}
- // The search failed because of an HTTP status code that we don't ignore for
- // this index. We end our search here.
- SimpleMetadataSearchOutcome::StatusCodeFailure(status_code) => {
- debug!("Indexes search failed because of status code failure: {status_code}");
- break;
- }
}
}
IndexFormat::Flat => {
@@ -522,17 +516,15 @@ impl RegistryClient {
let Some(status_code) = err.status() else {
return Err(ErrorKind::WrappedReqwestError(url, err).into());
};
- let decision =
- status_code_strategy.handle_status_code(status_code, index, capabilities);
- if let IndexStatusCodeDecision::Fail(status_code) = decision {
- if !matches!(
- status_code,
- StatusCode::UNAUTHORIZED | StatusCode::FORBIDDEN
- ) {
- return Err(ErrorKind::WrappedReqwestError(url, err).into());
+ match status_code_strategy.handle_status_code(status_code, index, capabilities)
+ {
+ IndexStatusCodeDecision::Ignore => {
+ Ok(SimpleMetadataSearchOutcome::NotFound)
+ }
+ IndexStatusCodeDecision::Fail(..) => {
+ Err(ErrorKind::WrappedReqwestError(url, err).into())
}
}
- Ok(SimpleMetadataSearchOutcome::from(decision))
}
// The package is unavailable due to a lack of connectivity.
@@ -998,18 +990,6 @@ pub(crate) enum SimpleMetadataSearchOutcome {
Found(OwnedArchive<SimpleMetadata>),
/// Simple metadata was not found
NotFound,
- /// A status code failure was encountered when searching for
- /// simple metadata and our strategy did not ignore it
- StatusCodeFailure(StatusCode),
-}
-
-impl From<IndexStatusCodeDecision> for SimpleMetadataSearchOutcome {
- fn from(item: IndexStatusCodeDecision) -> Self {
- match item {
- IndexStatusCodeDecision::Ignore => Self::NotFound,
- IndexStatusCodeDecision::Fail(status_code) => Self::StatusCodeFailure(status_code),
- }
- }
}
/// A map from [`IndexUrl`] to [`FlatIndexEntry`] entries found at the given URL, indexed by| // Package not found, so we will continue on to the next index (if there is one) | ||
| SimpleMetadataSearchOutcome::NotFound => {} | ||
| // The search failed because of an HTTP status code that we don't ignore for | ||
| // this index. We end our search here. |
There was a problem hiding this comment.
I think this should mention the assumption that the status code failure will be tracked by IndexCapabilities and surfaced via hints.
| if let IndexStatusCodeDecision::Fail(status_code) = decision { | ||
| if !matches!( | ||
| status_code, | ||
| StatusCode::UNAUTHORIZED | StatusCode::FORBIDDEN |
There was a problem hiding this comment.
I think this should mention that these specific codes are tracked via IndexCapabilities and surfaced via hints; otherwise, it's not really clear why these specific codes are ignored.
| NotFound, | ||
| /// A status code failure was encountered when searching for | ||
| /// simple metadata and our strategy did not ignore it | ||
| StatusCodeFailure(StatusCode), |
There was a problem hiding this comment.
I think I would find it clearer if we actually just made individual enum variants for Unauthorized and Forbidden, rather than allowing an arbitrary status code here. That way the expectations are encoded in the type system. We should make the invalid state (e.g., StatusCodeFailure(400)) unrepresentable.
|
I'm going to merge and kick the tires on the release. |
…y on authentication failure (#12805) uv’s default index strategy was designed with dependency confusion attacks in mind. [According to the docs](https://docs.astral.sh/uv/configuration/indexes/#searching-across-multiple-indexes), “if a package exists on an internal index, it should always be installed from the internal index, and never from PyPI”. Unfortunately, this is not true in the case where authentication fails on that internal index. In that case, uv will simply try the next index (even on the `first-index` strategy). This means that uv is not secure by default in this common scenario. This PR causes uv to stop searching for a package if it encounters an authentication failure at an index. It is possible to opt out of this behavior for an index with a new `pyproject.toml` option `ignore-error-codes`. For example: ``` [[tool.uv.index]] name = "my-index" url = "<index-url>" ignore-error-codes = [401, 403] ``` This will also enable users to handle idiosyncratic registries in a more fine-grained way. For example, PyTorch registries return a 403 when a package is not found. In this PR, we special-case PyTorch registries to ignore 403s, but users can use `ignore-error-codes` to handle similar behaviors if they encounter them on internal registries. Depends on #12651 Closes #9429 Closes #12362
…y on authentication failure (#12805) uv’s default index strategy was designed with dependency confusion attacks in mind. [According to the docs](https://docs.astral.sh/uv/configuration/indexes/#searching-across-multiple-indexes), “if a package exists on an internal index, it should always be installed from the internal index, and never from PyPI”. Unfortunately, this is not true in the case where authentication fails on that internal index. In that case, uv will simply try the next index (even on the `first-index` strategy). This means that uv is not secure by default in this common scenario. This PR causes uv to stop searching for a package if it encounters an authentication failure at an index. It is possible to opt out of this behavior for an index with a new `pyproject.toml` option `ignore-error-codes`. For example: ``` [[tool.uv.index]] name = "my-index" url = "<index-url>" ignore-error-codes = [401, 403] ``` This will also enable users to handle idiosyncratic registries in a more fine-grained way. For example, PyTorch registries return a 403 when a package is not found. In this PR, we special-case PyTorch registries to ignore 403s, but users can use `ignore-error-codes` to handle similar behaviors if they encounter them on internal registries. Depends on #12651 Closes #9429 Closes #12362
…y on authentication failure (#12805) uv’s default index strategy was designed with dependency confusion attacks in mind. [According to the docs](https://docs.astral.sh/uv/configuration/indexes/#searching-across-multiple-indexes), “if a package exists on an internal index, it should always be installed from the internal index, and never from PyPI”. Unfortunately, this is not true in the case where authentication fails on that internal index. In that case, uv will simply try the next index (even on the `first-index` strategy). This means that uv is not secure by default in this common scenario. This PR causes uv to stop searching for a package if it encounters an authentication failure at an index. It is possible to opt out of this behavior for an index with a new `pyproject.toml` option `ignore-error-codes`. For example: ``` [[tool.uv.index]] name = "my-index" url = "<index-url>" ignore-error-codes = [401, 403] ``` This will also enable users to handle idiosyncratic registries in a more fine-grained way. For example, PyTorch registries return a 403 when a package is not found. In this PR, we special-case PyTorch registries to ignore 403s, but users can use `ignore-error-codes` to handle similar behaviors if they encounter them on internal registries. Depends on #12651 Closes #9429 Closes #12362
|
Hi There, Just a quick question on the above - I have an internal package index which also returns 403 for missing packages. Is it possible to make the Furthermore, it seems to me that the workaround in |
You shouldn't need to include a token in the URL (and I agree you shouldn't include one in |
|
Yes - interestingly, I get the error |
|
Ok, would you mind opening an issue and including a reproduction and trace ( |
|
Should I put it here as it seems closely related, or would you rather a separate issue? |
|
I think a separate issue for now would be helpful. It's not clear to me these are the same problem. |
|
I've realised what the problem is just now (and it's sort of OK): as I was debugging, I left the This still leaves the outstanding question whether all this config could be saved to a |
|
Is this problem unique to 0.7.0 or do you also see it on 0.6.17? The |
|
The problem and enhancement request is both unique to 0.7.0. Before, I had all index and authentication info saved in |
uv’s default index strategy was designed with dependency confusion attacks in mind. According to the docs, “if a package exists on an internal index, it should always be installed from the internal index, and never from PyPI”. Unfortunately, this is not true in the case where authentication fails on that internal index. In that case, uv will simply try the next index (even on the
first-indexstrategy). This means that uv is not secure by default in this common scenario.This PR causes uv to stop searching for a package if it encounters an authentication failure at an index. It is possible to opt out of this behavior for an index with a new
pyproject.tomloptionignore-error-codes. For example:This will also enable users to handle idiosyncratic registries in a more fine-grained way. For example, PyTorch registries return a 403 when a package is not found. In this PR, we special-case PyTorch registries to ignore 403s, but users can use
ignore-error-codesto handle similar behaviors if they encounter them on internal registries.Depends on #12651
Closes #9429
Closes #12362